Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ds 5114 update the aws secrets provider to aws sdk v2 #76

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

arivankar-px
Copy link
Collaborator

@arivankar-px arivankar-px commented Jun 21, 2023

What this PR does / why we need it:
In this PR we have updated the packages used from aws sdk version 1 to aws sdk version 2. This has been made to maintain consistency in using the same aws sdk version across libopenstorage/secrets and portworx/pds-api repositories.

Which issue(s) this PR fixes (optional)
Closes #DS-5114

Special notes for your reviewer:

@arivankar-px arivankar-px marked this pull request as draft June 23, 2023 07:01
@arivankar-px arivankar-px marked this pull request as ready for review June 23, 2023 14:35
@arivankar-px arivankar-px requested a review from lprouza June 23, 2023 14:35
@arivankar-px arivankar-px requested a review from adityadani July 3, 2023 10:00
aws/credentials/credentials.go Outdated Show resolved Hide resolved
aws/credentials/credentials.go Show resolved Hide resolved
aws/credentials/credentials.go Outdated Show resolved Hide resolved
aws/credentials/credentials.go Outdated Show resolved Hide resolved
aws/aws_kms/aws_kms.go Outdated Show resolved Hide resolved
aws/credentials/credentials.go Outdated Show resolved Hide resolved
aws/aws_secrets_manager/aws_scm.go Outdated Show resolved Hide resolved
aws/aws_secrets_manager/aws_scm.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@adityadani adityadani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good once the integration tests are fixed.

}
providers = append(providers, ec2RoleProvider)

} else if runningOnEc2 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, does AWS not allow chaining of providers anymore? Previously we were able to add EnvProvider and RoleProvider together. With this change we won't since each provider is in its own if condition.

@@ -107,18 +108,27 @@ func (a *awsSecretTest) TestListSecrets(t *testing.T) error {

func (a *awsSecretTest) TestDeleteSecret(t *testing.T) error {
// Delete of a key that exists should succeed
err := a.s.DeleteSecret(a.secretIdWithData, nil)
keyContext := make(map[string]string)
keyContext[SecretRetentionPeriodInDaysKey] = "7"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you ad a retention period then the subsequent GetSecret check is going to fail ? The integration tests are failing for SecretsManager. Can you check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants